From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: Mask bit support's API
Date: Tue, 23 Nov 2010 08:17:28 +0200
Message-ID: <4CEB5C78.4060408@redhat.com>
References: <201011231409.52666.sheng.yang@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Marcelo Tosatti <mtosatti@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>, kvm@vger.kernel.org
To: "Yang, Sheng" <sheng.yang@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:37654 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751619Ab0KWGRd (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 23 Nov 2010 01:17:33 -0500
In-Reply-To: <201011231409.52666.sheng.yang@intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 11/23/2010 08:09 AM, Yang, Sheng wrote:
> Hi Avi,
>
> I've purposed the following API for mask bit support.
>
> The main point is, QEmu can know which entries are enabled(by pci_enable_msix()).
> And for enabled entries, kernel own it, including MSI data/address and mask
> bit(routing table and mask bitmap). QEmu should use KVM_GET_MSIX_ENTRY ioctl to
> get them(and it can sync with them if it want to do so).
>
> Before entries are enabled, QEmu can still use it's own MSI table(because we
> didn't contain these kind of information in kernel, and it's unnecessary for
> kernel).
>
> The KVM_MSIX_FLAG_ENTRY flag would be clear if QEmu want to query one entry didn't
> exist in kernel - or we can simply return -EINVAL for it.
>
> I suppose it would be rare for QEmu to use this interface to get the context of
> entry(the only case I think is when MSI-X disable and QEmu need to sync the
> context), so performance should not be an issue.
>
> What's your opinion?
>
> >  #define KVM_GET_MSIX_ENTRY        _IOWR(KVMIO,  0x7d, struct kvm_msix_entry)

Need SET_MSIX_ENTRY for live migration as well.

> >  #define KVM_UPDATE_MSIX_MMIO      _IOW(KVMIO,  0x7e, struct kvm_msix_mmio)
> >
> >  #define KVM_MSIX_TYPE_ASSIGNED_DEV      1
> >
> >  #define KVM_MSIX_FLAG_MASKBIT           (1<<  0)
> >  #define KVM_MSIX_FLAG_QUERY_MASKBIT     (1<<  0)
> >  #define KVM_MSIX_FLAG_ENTRY             (1<<  1)
> >  #define KVM_MSIX_FLAG_QUERY_ENTRY       (1<<  1)
> >

Why is there a need for the flag?  If we simply get/set entire entries, 
that includes the mask bits?

What about the pending bits?

> >  struct kvm_msix_entry {
> >          __u32 id;
> >          __u32 type;
> >          __u32 entry; /* The index of entry in the MSI-X table */
> >          __u32 flags;
> >          __u32 query_flags;
> >          union {
> >                  struct {
> >                          __u32 addr_lo;
> >                          __u32 addr_hi;
> >                          __u32 data;

Isn't the mask bit in the last word?  Or maybe I'm confused about the 
format.

>
> >                  } msi_entry;
> >                  __u32 reserved[12];
> >          };
> >  };
> >
> >  #define KVM_MSIX_MMIO_FLAG_REGISTER     (1<<  0)
> >  #define KVM_MSIX_MMIO_FLAG_UNREGISTER   (1<<  1)
> >  #define KVM_MSIX_MMIO_FLAG_MASK         0x3
> >
> >  struct kvm_msix_mmio {
> >          __u32 id;
> >          __u32 type;
> >          __u64 base_addr;
> >          __u32 max_entries_nr;
> >          __u32 flags;
> >          __u32 reserved[6];
> >  };

Also need a new exit reason to tell userspace that an msix entry has 
changed, so userspace can update mappings.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.