Re: [Qemu-devel] [PATCH v5 2/3] target-i386: reserve RCRB mmio space in ACPI DSDT table

[Paulo Alcantara <pcacjr@zytor.com>]

On Wed, 24 Jun 2015 17:11:26 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jun 22, 2015 at 08:10:28PM -0300, Paulo Alcantara wrote:
> > This block is mapped into memory space, using the Root Complex Base
> > Address (RCBA) register of the PCI-to-LPC bridge. Accesses in this
> > space must be limited to 32-(DW) bit quantities. Burst accesses are
> > not allowed.
> > 
> > All Chipset Configuration Registers are located in this 16KiB space.
> > 
> > Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> 
> Can you confirm things just work for you if you drop this patch from
> the series?

Hi Michael,

Yes, everything works. I still see no point of adding it to SSDT
because OVMF is already setting RCBA to that fixed address and so
SeaBIOS once my patch is applied. Therefore, there is no no such thing
like "dynamically programmed by BIOS" either. Although, my host and
probably yours define the same address in DSDT.

Given that UEFI and a BIOS implementations are using the same fixed
address, this patch was done in a hope of not letting the ACPI-aware OS
using the same address for another BAR or something else thus avoiding
conflicts.

Please, let me know if I'm missing something here. If you want, I can
send v6 with this patch removed completely.

(BTW, sorry for the delay. It's holiday here in Brazil and, you know,
real life happens sometimes :-) )

Thanks,

Paulo

> 
> > ---
> > v1 -> v2:
> >   * s/PDRC/CCR/ for clarity and match ICH9 spec
> >   * remove unnecessary OperationRegion for RCRB
> > 
> > v2 -> v3: (no changes)
> > 
> > v3 -> v4:
> >   * quote RCRB description from ICH9 spec to commit log
> >   * fix indentation issue in _CRS() method declaration
> >   * create hw/i386/ich9-cc.h for chipset configuration register
> > values and use them in ASL
> > 
> > v4 -> v5:
> >   * prefix macros in ich9-cc.h with "ICH9_" for better readability
> > and make use of them in CCR device definition
> > ---
> >  hw/i386/q35-acpi-dsdt.dsl     |  16 ++++++++++++++++
> >  include/hw/i386/ich9-cc.h     |  15 +++++++++++++++
> >  tests/acpi-test-data/q35/DSDT | Bin 7666 -> 7723 bytes
> >  3 files changed, 31 insertions(+)
> >  create mode 100644 include/hw/i386/ich9-cc.h
> > 
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 16eaca3..8f4bb6a 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -114,6 +114,22 @@ DefinitionBlock (
> >          }
> >      }
> >  
> > +#include "hw/i386/ich9-cc.h"
> > +
> > +/****************************************************************
> > + * Chipset Configuration Registers
> > + ****************************************************************/
> > +Scope(\_SB.PCI0) {
> > +    Device (CCR) {
> > +        Name (_HID, EISAID("PNP0C02"))
> > +        Name (_UID, 1)
> > +
> > +        Name (_CRS, ResourceTemplate() {
> > +            Memory32Fixed(ReadWrite, ICH9_RCBA_BASE_ADDR,
> > ICH9_RCRB_SIZE)
> > +        })
> > +    }
> > +}
> > +
> >  #include "acpi-dsdt-hpet.dsl"
> >  
> >  
> > diff --git a/include/hw/i386/ich9-cc.h b/include/hw/i386/ich9-cc.h
> > new file mode 100644
> > index 0000000..d4918ff
> > --- /dev/null
> > +++ b/include/hw/i386/ich9-cc.h
> > @@ -0,0 +1,15 @@
> > +/*
> > + * QEMU ICH9 Chipset Configuration Registers
> > + *
> > + * Copyright (c) 2015 Paulo Alcantara <pcacjr@zytor.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2
> > or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#ifndef HW_ICH9_CC_H
> > +#define HW_ICH9_CC_H
> > +
> > +#define ICH9_RCBA_BASE_ADDR    0xfed1c000
> > +#define ICH9_RCRB_SIZE         0x00004000
> > +
> > +#endif /* HW_ICH9_CC_H */
> > diff --git a/tests/acpi-test-data/q35/DSDT
> > b/tests/acpi-test-data/q35/DSDT index
> > 4723e5954dccb00995ccaf521b7daf6bf15cf1d4..f3bda7b54ea6d669b1498d9380e7781207fb6e49
> > 100644 GIT binary patch delta 81
> > zcmexlz1oJ$CD<iITaJN&F>xbTJfnq$UVN}qe1Nm3L3ERjvvW{9N4$rp3y<Rk9wv_X
> > lP)`>|j(F#wU_n7HzBWz<Mur0y|1mf)FjO*Z&S3140RVI`6(s-w
> > 
> > delta 24
> > gcmZ2&^U0daCD<k8lPm)R<DrdQ@r;`nF?PxT0Bl$YHUIzs
> > 
> > -- 
> > 2.1.0
> 



-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.