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

[Jan Beulich]

>>> On 05.06.15 at 18:41, <address@hidden> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> >>> On 05.06.15 at 13:32, <address@hidden> 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.

Jan