Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask operation from guests

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, November 25, 2013 5:30 PM
> To: Wu, Feng
> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> Subject: RE: [PATCH v4] x86: properly handle MSI-X unmask operation from
> guests
> 
> >>> On 23.11.13 at 02:40, "Wu, Feng" <feng.wu@xxxxxxxxx> wrote:
> >> > +struct pending_msix_unmask_info
> >> > +{
> >> > +    unsigned long ctrl_address;
> >> > +    bool_t valid;
> >> > +};
> >> > +
> >> >  struct arch_vcpu
> >> >  {
> >> >      /*
> >> > @@ -439,6 +445,8 @@ struct arch_vcpu
> >> >
> >> >      /* A secondary copy of the vcpu time info. */
> >> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> >> > +
> >> > +    struct pending_msix_unmask_info pending_msix_unmask;
> >>
> >> I don't think you need a separate boolean here - for one I don't
> >> think the address can reasonably be zero, and even if you have
> >> the bottom two bits available (as it being 4-byte aligned gets
> >> checked before you consume it).
> >
> >
> > The boolean variant "valid", which is set in msixtbl_write(), means whether
> > there is a
> > msix pending unmask, if there is, just clean this flag and unmask the msix
> > in hardware,
> > otherwise we just do nothing. If removing "valid", how can I know whether
> > there is a
> > msix pending unmask operation ? Thanks you!
> 
> Quoting your patch:
> 
> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long
> address,
> 
>      /* exit to device model if address/data has been modified */
>      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> +    {
> +        v->arch.pending_msix_unmask.valid = 1;
> +        v->arch.pending_msix_unmask.ctrl_address = address;
>          goto out;
> +    }
> 
>      virt = msixtbl_addr_to_virt(entry, address);
>      if ( !virt )
> 
> You always set "ctrl_address" together with "valid". Hence, as long
> as "ctrl_address" is guaranteed to be non-zero (either a apriori
> knowledge, or by setting one of the low to bits, which aren't used
> for other purposes), you can then check "ctrl_address" to be non-
> zero instead of checking "valid".

Yes, this is a good idea. Thanks for the clear explanation!

> 
> Jan


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