Re: [Xen-devel] [PATCH v6 11/18] vt-d: Add API to update IRTE when VT-d PI is used

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.15 at 07:24, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Friday, September 04, 2015 11:11 PM
>> >>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:
>> > --- a/xen/drivers/passthrough/vtd/intremap.c
>> > +++ b/xen/drivers/passthrough/vtd/intremap.c
>> > @@ -899,3 +899,110 @@ void iommu_disable_x2apic_IR(void)
>> >      for_each_drhd_unit ( drhd )
>> >          disable_qinval(drhd->iommu);
>> >  }
>> > +
>> > +static void setup_posted_irte(struct iremap_entry *new_ire,
>> > +    const struct pi_desc *pi_desc, const uint8_t gvec)
>> > +{
>> > +    new_ire->post.urg = 0;
>> > +    new_ire->post.vector = gvec;
>> > +    new_ire->post.pda_l = virt_to_maddr(pi_desc) >> (32 - PDA_LOW_BIT);
>> > +    new_ire->post.pda_h = virt_to_maddr(pi_desc) >> 32;
>> > +
>> > +    new_ire->post.res_1 = 0;
>> > +    new_ire->post.res_2 = 0;
>> > +    new_ire->post.res_3 = 0;
>> > +    new_ire->post.res_4 = 0;
>> > +    new_ire->post.res_5 = 0;
>> > +
>> > +    new_ire->post.im = 1;
>> > +}
>> 
>> I think it would be better to just clear out the structure first. This
>> may then also allow for no longer naming all of the bitfield res_*
>> member, making it even more obvious that they're reserved. Or
>> wait - looking at the use below you seem to be updating the
>> descriptor. Why would you need to zero out reserved fields in
>> that case?
> 
> Here we first get the IRTE from hardware, which may be in
> remapped format, we still need some of the information in it
> after updating it to posted format, such as, fpd, sid, etc. So
> I cannot clear it here. Besides that, there are some fields which
> are needed in remapped format are defined as reserved in posted
> format, I need to clear the reserved field one by one if we get a
> remapped format from the hardware, here I just simply clear it
> for all the cases and it is not a frequent operations, I think it is
> not a big deal.

While perhaps indeed not a big deal performance wise, I would
suppose you at least partly agree that the code above looks
ugly. I'd much rather see made explicit which fields you re-use
than which (reserved) fields you clear. I.e. start out from a
zeroed structure, copy over all fields from the original which you
want to retain, and fill in any non-reserved fields you have to
give new values.

Jan


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