Re: Mask bit support's API

[Avi Kivity <avi@redhat.com>]

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.