[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH] xen/pass-through: ROM BAR handling adjustments |
Date: |
Mon, 8 Jun 2015 12:35:28 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Mon, 8 Jun 2015, Jan Beulich wrote:
> >>> 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.
I prefer if you resubmit, thanks!