Re: [Xen-devel] [PATCH] xen vtd : set msi guest_masked 0 by default

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.16 at 09:12, <changjzh@xxxxxxxxx> wrote:
> 2016-01-26 20:56 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
> 
>> >>> On 26.01.16 at 02:34, <changjzh@xxxxxxxxx> wrote:
>> > There are some problems when msi guest_masked is set to 1 by default.
>> > When guest os is windows 2008 r2 server,
>> > the device(eg X540-AT2 vf) is not initialized correctly.
>> > Host will always receive message like this :"VF Reset msg received from
>> vf".
>> > Guest has network connectivity issues,
>> > and can not correctly receive/send the packet.
>> > So, guest_masked is set to 0 by default.
>>
>> You describe a problem and half of your change, but there's no
>> connection between the two: What is actually wrong with current
>> behavior (matching the hardware's - MSI-X mask bits are set when
>> coming out of reset).
>>
>> > --- a/xen/arch/x86/msi.c
>> > +++ b/xen/arch/x86/msi.c
>> > @@ -512,7 +512,7 @@ void guest_mask_msi_irq(struct irq_desc *desc,
>> bool_t mask)
>> >
>> >  static unsigned int startup_msi_irq(struct irq_desc *desc)
>> >  {
>> > -    if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status &
>> IRQ_GUEST))) )
>> > +    if ( unlikely(!msi_set_mask_bit(desc, 0, 0) ))
>> >          WARN();
>> >      return 0;
>> >  }
>>
>> Whether this part can go under "set ... by default" is highly
>> questionable. Plus, while this affects MSI and MSI-X, ...
>>
>>  If irq is owned by guest,in function msi_set_mask_bit():
> ...
> bool_t flag = host || guest; //The flag should be true.
> ...
>  writel(flag, entry->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> ...
> PCI device can not generate interrrupt.
> windows guest can not change vector_ctrl_mask, guest os get abnormal status
> of nic.

Here and below - I'm sorry, I do not understand what you're trying
to tell us, or how this is meant to extend beyond the original (too
vague) description of your change. Among unclear things is why
"windows guest can not change vector_ctrl_mask" - you again just
make statements without dealing with any of the why-s.

Please can you try to explain things by matching operations done
by the guest, qemu, and the hypervisor to the effect they have
on the state of the mask bit, and then point out which of those
steps needs changing or doesn't work as intended?

Jan

>> > @@ -972,7 +972,7 @@ static int msix_capability_init(struct pci_dev *dev,
>> >          entry->msi_attrib.entry_nr = msi->entry_nr;
>> >          entry->msi_attrib.maskbit = 1;
>> >          entry->msi_attrib.host_masked = 1;
>> > -        entry->msi_attrib.guest_masked = 1;
>> > +        entry->msi_attrib.guest_masked = 0;
>> >          entry->msi_attrib.pos = pos;
>> >          entry->irq = msi->irq;
>> >          entry->dev = dev;
>>
>> ... this change affect MSI-X only, and doing some guessing from
>> what you write above I suspect you only really tested one of the
>> two cases.
>>
>> So while the change _may_ be necessary, you'll need to do a
>> better job at explaining why you what you do.
>>
> Msi guest_masked is set to 0 in the original code, only msi-x guest_masked
> is modifed in msix_capability_init() function by patch.
> 
>>
>> Jan
>>
>>
> This issue appears after commited the variable guest_mask.
> Initialization operations of pci device may be changed in windows
> guest,or Xen need to change the initial state of vtd pci device.
> -- 
> Jianzhong,Chang




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel