Re: [Xen-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 8 Jun 2015, Jan Beulich wrote:
> >>> On 05.06.15 at 18:41, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> >>> On 05.06.15 at 13:32, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> >> --- a/hw/xen/xen_pt.c
> >> >> +++ b/hw/xen/xen_pt.c
> >> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID
> >> >>  
> >> >>      /* check unused BAR register */
> >> >>      index = xen_pt_bar_offset_to_index(addr);
> >> >> -    if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) &&
> >> >> +    if ((index >= 0) && (val != 0) &&
> >> >> +        (((index != PCI_ROM_SLOT) ?
> >> >> +          val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != 
> > XEN_PT_BAR_ALLF) &&
> >> > 
> >> > The change seems looks good and in line with the commit message. But
> >> > this if statement looks like acrobatic circus to me now.
> >> 
> >> I think the alternative of splitting it up into multiple if()-s would not
> >> be any better readable.
> > 
> > Would you be OK if I rewrote the statement as follows?
> > 
> >     if ((index >= 0) && (val != 0)) {
> >         uint32_t vu;
> >         
> >         if (index == PCI_ROM_SLOT) {
> >             vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK;
> >         } else {
> >             vu = val;
> >         }
> > 
> >         if ((vu != XEN_PT_BAR_ALLF) &&
> >             (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) {
> >             XEN_PT_WARN(d, "Guest attempt to set address to unused Base 
> > Address "
> >                     "Register. (addr: 0x%02x, len: %d)\n", addr, len);
> >         }
> >     }
> 
> Actually I agree that this indeed looks better. If I were to make the
> change, I'd do
> 
>     if ((index >= 0) && (val != 0)) {
>         uint32_t vu = val;
>         
>         if (index == PCI_ROM_SLOT) {
>             vu |= (uint32_t)~PCI_ROM_ADDRESS_MASK;
>         }
> 
>         if ((vu != XEN_PT_BAR_ALLF) &&
>         ...
> 
> though. But if you're going to do the edit without wanting me to
> re-submit, I'll certainly leave this to you. Just let me know which
> way you prefer it to be handled.

I prefer if you resubmit, thanks!

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