[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/8] hw/intc: GICv3 ITS command queue framework
|
From: |
shashi . mallela |
|
Subject: |
Re: [PATCH v2 3/8] hw/intc: GICv3 ITS command queue framework |
|
Date: |
Thu, 29 Apr 2021 19:38:35 -0400 |
On Mon, 2021-04-19 at 11:30 +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 03:41, Shashi Mallela <
> shashi.mallela@linaro.org> wrote:
> > Added functionality to trigger ITS command queue processing on
> > write to CWRITE register and process each command queue entry to
> > identify the command type and handle commands like MAPD,MAPC,SYNC.
> >
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > ---
> > hw/intc/arm_gicv3_its.c | 347
> > +++++++++++++++++++++++++++++++++++++++
> > hw/intc/gicv3_internal.h | 41 +++++
> > 2 files changed, 388 insertions(+)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 81373f049d..fcac33c836 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -28,6 +28,347 @@ struct GICv3ITSClass {
> > void (*parent_reset)(DeviceState *dev);
> > };
> >
> > +static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
> > +{
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + uint64_t rdbase;
> > + uint64_t value;
> > + bool pta = false;
> > + MemTxResult res = MEMTX_OK;
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + offset += NUM_BYTES_IN_DW;
> > +
> > + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED,
> > &res);
>
> If the ITS fails to read or write DMA, it does not report that
> by provoking a bus error for the CPU's write to GITS_CWRITER,
> which is what will happen if you just propagate up the MemTxResult
> from this DMA operation back through the register write function
> to its caller. The spec doesn't seem to define a particular behaviour
> for "I couldn't read the command out of the command queue", but
> it would seem sensible to pick one of the options from the "Command
> errors"
> section: ignore the command, or stall the command queue ("treat as
> valid" is the other option there but doesn't seem relevant here).
>
> "Stall" is probably best as otherwise we might loop forever through
> a region of unreadable addresses.
>
> have implemented "stall" indication for all failing dma read/write
calls(based on the return value of each call)
>
> > + if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {
> > + /*
> > + * only bits[47:16] are considered instead of bits [51:16]
> > + * since with a physical address the target address must
> > be
> > + * 64KB aligned
> > + */
> > + rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;
> > + pta = true;
> > + } else {
> > + rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;
> > + }
> > +
> > + if (!pta && (rdbase < (s->gicv3->num_cpu))) {
> > + /*
> > + * Current implementation makes a blocking synchronous
> > call
> > + * for every command issued earlier,hence the internal
> > state
> > + * is already consistent by the time SYNC command is
> > executed.
> > + */
> > + }
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + return res;
> > +}
>
> TODO: review process_mapd and process_mapc
>
> > +
> > +static void update_cte(GICv3ITSState *s, uint16_t icid, uint64_t
> > cte)
> > +{
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + uint64_t value;
> > + uint8_t page_sz_type;
> > + uint64_t l2t_addr;
> > + bool valid_l2t;
> > + uint32_t l2t_id;
> > + uint32_t page_sz = 0;
> > + uint32_t max_l2_entries;
>
> I think it's worth having a comment here:
>
> /*
> * The specification defines the format of level 1 entries of a
> * 2-level table, but the format of level 2 entries and the format
> * of flat-mapped tables is IMPDEF.
> */
>
> Q: why have you chosen to make the level-2 and flatmap entries
> 64 bits here ? They only need to contain the valid bit plus a
> 16 bit processor number. (We know the processor number fits in 16
> bits because the GICR_TYPER.Processor_Number field in the
> redistributor
> is that large.) Is there something I'm missing in the spec that makes
> 64-bit entries a good choice anyway ?
>
> I chose 64 bits to be able to have the data structure ready for both
the processor number and RD_base address possibilities, in case we add
RD_Base address support in the future
> > +
> > + if (s->ct.indirect) {
> > + /* 2 level table */
> > + page_sz_type = FIELD_EX64(s->baser[1], GITS_BASER,
> > PAGESIZE);
> > +
> > + if (page_sz_type == 0) {
> > + page_sz = GITS_ITT_PAGE_SIZE_0;
> > + } else if (page_sz_type == 1) {
> > + page_sz = GITS_ITT_PAGE_SIZE_1;
> > + } else if (page_sz_type == 2) {
> > + page_sz = GITS_ITT_PAGE_SIZE_2;
> > + }
>
> page_sz_type == 3 has a defined meaning: must be treated like 2.
>
> If we're caching stuff in s->ct, maybe we should cache page_sz too ?
>
> > +
> > + l2t_id = icid / (page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > + value = address_space_ldq_le(as,
> > + s->ct.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > NULL);
> > +
> > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid_l2t) {
> > + max_l2_entries = page_sz / s->ct.entry_sz;
> > +
> > + l2t_addr = (value >> page_sz_type) &
> > + ((1ULL << (51 - page_sz_type)) - 1);
>
> The spec text could be more clearly worded, but I don't think this is
> how
> the physaddr is encoded in the L1 descriptor. I think that the intent
> is
> that bits [51:0] are the physical address, with bits [N-1:0] of that
> being 0
> because the L2 table must be page aligned. Your intepretation (that
> the
> physaddr bit 0 is in bit N of the descriptor) wouldn't allow L2
> tables
> to be anywhere in the address space.
>
> > +
> > + address_space_write(as, l2t_addr +
> > + ((icid % max_l2_entries) *
> > GITS_CTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > + &cte, sizeof(cte));
>
> Writing the CTE word using address_space_write() like this will
> do the wrong thing on big-endian hosts. address_space_write() is the
> "write a string of bytes to guest memory" function, so it will write
> the uint64_t in whatever the host's byte order is. You want a fixed
> byte order, so use address_space_ldq_le() or similar.
>
> > + }
> > + } else {
> > + /* Flat level table */
> > + address_space_write(as, s->ct.base_addr + (icid *
> > GITS_CTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, &cte,
> > + sizeof(cte));
> > + }
> > +}
>
> I feel like there's a fair amount of code duplication between
> get_cte(), get_dte(), update_cte() and update_dte() which could
> perhaps be refactored out.
>
> > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> > +{
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + uint16_t icid;
> > + uint64_t rdbase;
> > + bool valid;
> > + bool pta = false;
> > + MemTxResult res = MEMTX_OK;
> > + uint64_t cte_entry;
> > + uint64_t value;
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + offset += NUM_BYTES_IN_DW;
> > +
> > + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, &res);
> > +
> > + icid = value & ICID_MASK;
> > +
> > + if (FIELD_EX64(s->typer, GITS_TYPER, PTA)) {
> > + /*
> > + * only bits[47:16] are considered instead of bits [51:16]
> > + * since with a physical address the target address must
> > be
> > + * 64KB aligned
> > + */
> > + rdbase = (value >> RDBASE_OFFSET) & RDBASE_MASK;
> > + pta = true;
> > + } else {
> > + rdbase = (value >> RDBASE_OFFSET) & RDBASE_PROCNUM_MASK;
> > + }
>
> GITS_TYPER.PTA is a read-only bit where we report to the guest how
> our implementation behaves (ie whether these are processor numbers
> or redistributor base addresses). We should implement one thing,
> whichever is the easy one to implement for us, not both.
>
> The specification is giving implementations a choice here because
> there are different ways an ITS-to-redistributor interface might
> work:
> * the ITS might talk to the redistributor by literally writing to
> its registers via DMA (in which case setting GITS_TYPER.PTA makes
> its life easier)
> * the ITS might have its own backdoor or internal communications
> channel to the redistributors (in which case it wants to know a
> CPU number)
>
> Looking forward, in patch 6 you add a comment:
> * Current implementation only supports rdbase == procnum
> * Hence rdbase physical address is ignored
>
> So we're the second type of implementation. We should set
> GITS_TYPER.PTA=0, and then this code can simply assume that
> the data is always the "it's a CPU number" format.
>
> > +
> > + valid = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid) {
> > + if ((icid > s->ct.max_collids) || (!pta &&
> > + (rdbase > s->gicv3->num_cpu))) {
> > + if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {
> > + /* Generate System Error here if supported */
> > + }
>
> Again, GITS_TYPER.SEIS is a bit which we use to tell the guest
> whether
> we're going to generate system errors or not; it's not a bit which
> we read in order to decide whether to generate them. For QEMU, the
> CPU itself doesn't have system-error support properly yet, so for
> us SEIS should be zero. In the ITS code we thus have two choices:
> (1) just don't put anything in the code
> (2) add a function call to an empty function called something
> like its_raise_serror() which just has a comment noting that
> it's a placeholder to mark where we would need to raise an
> SError
> if we wanted to support it
>
> I think I would favour option 1.
>
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: invalid collection table attributes "
> > + "icid %d rdbase %lu\n", __func__, icid, rdbase);
>
> This kind of LOG_GUEST_ERROR logging doesn't need to include the
> __func__ string:
> saying "ITS MAPC command" is more comprehensible to a reader of the
> log who doesn't necessarily want to look into the QEMU source code
> than saying "process_mapc".
>
> > + /*
> > + * in this implementation,in case of error
> > + * we ignore this command and move onto the next
> > + * command in the queue
> > + */
> > + } else {
> > + if (s->ct.valid) {
> > + /* add mapping entry to collection table */
> > + cte_entry = (valid & VALID_MASK) |
> > + (pta ? ((rdbase & RDBASE_MASK) <<
> > 1ULL) :
> > + ((rdbase & RDBASE_PROCNUM_MASK) <<
> > 1ULL));
> > +
> > + update_cte(s, icid, cte_entry);
> > + }
>
> Put the s->ct.valid check inside update_cte(). Pass the
> various parameters -- valid, pta, rdbase -- to update_cte()
> and have it do the assembly into a word. That way all the code
> dealing with the specific format of the table is inside that
> function,
> rather than being partly here and partly there.
>
> > + }
> > + } else {
> > + if (s->ct.valid) {
> > + /* remove mapping entry from collection table */
> > + cte_entry = 0;
> > +
> > + update_cte(s, icid, cte_entry);
> > + }
> > + }
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + offset += NUM_BYTES_IN_DW;
> > +
> > + return res;
> > +}
> > +
> > +static void update_dte(GICv3ITSState *s, uint32_t devid, uint64_t
> > dte)
> > +{
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + uint64_t value;
> > + uint8_t page_sz_type;
> > + uint64_t l2t_addr;
> > + bool valid_l2t;
> > + uint32_t l2t_id;
> > + uint32_t page_sz = 0;
> > + uint32_t max_l2_entries;
> > +
> > + if (s->dt.indirect) {
> > + /* 2 level table */
> > + page_sz_type = FIELD_EX64(s->baser[0], GITS_BASER,
> > PAGESIZE);
> > +
> > + if (page_sz_type == 0) {
> > + page_sz = GITS_ITT_PAGE_SIZE_0;
> > + } else if (page_sz_type == 1) {
> > + page_sz = GITS_ITT_PAGE_SIZE_1;
> > + } else if (page_sz_type == 2) {
> > + page_sz = GITS_ITT_PAGE_SIZE_2;
> > + }
> > +
> > + l2t_id = devid / (page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > + value = address_space_ldq_le(as,
> > + s->dt.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > NULL);
> > +
> > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid_l2t) {
> > + max_l2_entries = page_sz / s->dt.entry_sz;
> > +
> > + l2t_addr = (value >> page_sz_type) &
> > + ((1ULL << (51 - page_sz_type)) - 1);
> > +
> > + address_space_write(as, l2t_addr +
> > + ((devid % max_l2_entries) *
> > GITS_DTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, &dte,
> > sizeof(dte));
> > + }
> > + } else {
> > + /* Flat level table */
> > + address_space_write(as, s->dt.base_addr + (devid *
> > GITS_DTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, &dte,
> > sizeof(dte));
> > + }
> > +}
> > +
> > +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,
> > + uint32_t offset)
> > +{
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + uint32_t devid;
> > + uint8_t size;
> > + uint64_t itt_addr;
> > + bool valid;
> > + MemTxResult res = MEMTX_OK;
> > + uint64_t dte_entry = 0;
> > +
> > + devid = (value >> DEVID_OFFSET) & DEVID_MASK;
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, &res);
> > + size = (value & SIZE_MASK);
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, &res);
> > + itt_addr = (value >> ITTADDR_OFFSET) & ITTADDR_MASK;
> > +
> > + valid = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid) {
> > + if ((devid > s->dt.max_devids) ||
> > + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
> > + if (FIELD_EX64(s->typer, GITS_TYPER, SEIS)) {
> > + /* Generate System Error here if supported */
> > + }
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: invalid device table attributes "
> > + "devid %d or size %d\n", __func__, devid, size);
> > + /*
> > + * in this implementation,in case of error
> > + * we ignore this command and move onto the next
> > + * command in the queue
> > + */
> > + } else {
> > + if (s->dt.valid) {
> > + /* add mapping entry to device table */
> > + dte_entry = (valid & VALID_MASK) |
> > + ((size & SIZE_MASK) << 1U) |
> > + ((itt_addr & ITTADDR_MASK) << 6ULL);
> > +
> > + update_dte(s, devid, dte_entry);
> > + }
> > + }
> > + } else {
> > + if (s->dt.valid) {
> > + /* remove mapping entry from device table */
> > + dte_entry = 0;
> > + update_dte(s, devid, dte_entry);
> > + }
> > + }
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + offset += NUM_BYTES_IN_DW;
> > +
> > + return res;
> > +}
>
> Review comments for process_mapc() and update_cte() apply also to
> process_mapd() and update_dte().
>
> > +
> > +/*
> > + * Current implementation blocks until all
> > + * commands are processed
> > + */
> > +static MemTxResult process_cmdq(GICv3ITSState *s)
>
> This should return 'void' (see comments earlier).
>
> > +{
> > + uint32_t wr_offset = 0;
> > + uint32_t rd_offset = 0;
> > + uint32_t cq_offset = 0;
> > + uint64_t data;
> > + AddressSpace *as = &s->gicv3->sysmem_as;
> > + MemTxResult res = MEMTX_OK;
> > + uint8_t cmd;
> > +
> > + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET);
> > +
> > + if (wr_offset > s->cq.max_entries) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: invalid write offset "
> > + "%d\n", __func__, wr_offset);
> > + res = MEMTX_ERROR;
> > + return res;
> > + }
> > +
> > + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET);
> > +
> > + while (wr_offset != rd_offset) {
> > + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE);
> > + data = address_space_ldq_le(as, s->cq.base_addr +
> > cq_offset,
> > + MEMTXATTRS_UNSPECIFIED,
> > &res);
>
> I think I would just read all four quadwords of the command here
> into a local uint64_t data[4], and pass that to the process_$cmd()
> functions. That saves those functions doing the loads and handling
> possible errors doing the loads themselves.
>
> There are 2 reasons i didn't pick reading all 4 quadwords at once:-
> 1) not all commands have valid quadword data making the dma call
> for such RES0 quadwords insignificant
> 2) Since we are now stalling on any dma read/write,in case we fail
> during RES0 quadword read ,stalling doesnt seem significant
>
> Have optimised most of the similar-looking boilerplate code though
> > + cmd = (data & CMD_MASK);
> > +
> > + switch (cmd) {
> > + case GITS_CMD_INT:
> > + break;
> > + case GITS_CMD_CLEAR:
> > + break;
> > + case GITS_CMD_SYNC:
> > + res = process_sync(s, cq_offset);
> > + break;
> > + case GITS_CMD_MAPD:
> > + res = process_mapd(s, data, cq_offset);
> > + break;
> > + case GITS_CMD_MAPC:
> > + res = process_mapc(s, cq_offset);
> > + break;
> > + case GITS_CMD_MAPTI:
> > + break;
> > + case GITS_CMD_MAPI:
> > + break;
> > + case GITS_CMD_DISCARD:
> > + break;
> > + default:
> > + break;
> > + }
> > + if (res == MEMTX_OK) {
> > + rd_offset++;
> > + rd_offset %= s->cq.max_entries;
> > + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET,
> > rd_offset);
> > + } else {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: %x cmd processing failed!!\n", __func__,
> > cmd);
> > + break;
> > + }
> > + }
> > + return res;
> > +}
> > +
> > static bool extract_table_params(GICv3ITSState *s, int index)
> > {
> > uint16_t num_pages = 0;
> > @@ -282,6 +623,9 @@ static MemTxResult its_writel(GICv3ITSState *s,
> > hwaddr offset,
> > break;
> > case GITS_CWRITER:
> > s->cwriter = deposit64(s->cwriter, 0, 32, value);
> > + if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s-
> > >creadr)) {
> > + result = process_cmdq(s);
> > + }
> > break;
> > case GITS_CWRITER + 4:
> > s->cwriter = deposit64(s->cwriter, 32, 32, value);
> > @@ -416,6 +760,9 @@ static MemTxResult its_writell(GICv3ITSState
> > *s, hwaddr offset,
> > break;
> > case GITS_CWRITER:
> > s->cwriter = value;
> > + if ((s->ctlr & ITS_CTLR_ENABLED) && (s->cwriter != s-
> > >creadr)) {
> > + result = process_cmdq(s);
> > + }
>
> I would move the check on CTLR.ENABLED into process_cmdq() itself
> (which is already doing a writer != reader check).
>
> > break;
> > case GITS_TYPER:
> > case GITS_CREADR:
> > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> > index b7701e8ca5..7e1ff426ef 100644
> > --- a/hw/intc/gicv3_internal.h
> > +++ b/hw/intc/gicv3_internal.h
> > @@ -253,6 +253,12 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3)
> > FIELD(GITS_CBASER, INNERCACHE, 59, 3)
> > FIELD(GITS_CBASER, VALID, 63, 1)
> >
> > +FIELD(GITS_CREADR, STALLED, 0, 1)
> > +FIELD(GITS_CREADR, OFFSET, 5, 15)
> > +
> > +FIELD(GITS_CWRITER, RETRY, 0, 1)
> > +FIELD(GITS_CWRITER, OFFSET, 5, 15)
> > +
> > FIELD(GITS_CTLR, ENABLED, 0, 1)
> > FIELD(GITS_CTLR, QUIESCENT, 31, 1)
> >
> > @@ -284,6 +290,41 @@ FIELD(GITS_TYPER, CIL, 36, 1)
> > #define L1TABLE_ENTRY_SIZE 8
> >
> > #define GITS_CMDQ_ENTRY_SIZE 32
> > +#define NUM_BYTES_IN_DW 8
> > +
> > +#define CMD_MASK 0xff
> > +
> > +/* ITS Commands */
> > +#define GITS_CMD_CLEAR 0x04
> > +#define GITS_CMD_DISCARD 0x0F
> > +#define GITS_CMD_INT 0x03
> > +#define GITS_CMD_MAPC 0x09
> > +#define GITS_CMD_MAPD 0x08
> > +#define GITS_CMD_MAPI 0x0B
> > +#define GITS_CMD_MAPTI 0x0A
> > +#define GITS_CMD_SYNC 0x05
> > +
> > +/* MAPC command fields */
> > +#define ICID_LEN 16
> > +#define ICID_MASK ((1U << ICID_LEN) - 1)
> > +#define RDBASE_LEN 32
> > +#define RDBASE_OFFSET 16
> > +#define RDBASE_MASK ((1ULL << RDBASE_LEN) - 1)
>
> If you want to define these by hand, you could at least use the same
> suffixes that the FIELD() macro does: _SHIFT, _LENGTH and _MASK.
>
> > +#define RDBASE_PROCNUM_LEN 16
> > +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LEN) -
> > 1)
> > +
> > +#define DEVID_OFFSET 32
> > +#define DEVID_LEN 32
> > +#define DEVID_MASK ((1ULL << DEVID_LEN) - 1)
> > +
> > +/* MAPD command fields */
> > +#define ITTADDR_LEN 44
> > +#define ITTADDR_OFFSET 8
> > +#define ITTADDR_MASK ((1ULL << ITTADDR_LEN) - 1)
> > +#define SIZE_MASK 0x1f
> > +
> > +#define VALID_SHIFT 63
> > +#define VALID_MASK 0x1
> >
> > /**
> > * Default features advertised by this version of ITS
> > --
> > 2.27
>
> thanks
> -- PMM