* [PATCH 1/3] ppc: Check partition and process table alignment
2022-06-20 20:27 [PATCH 0/3] ppc: Check for bad Radix configs Leandro Lupori
@ 2022-06-20 20:27 ` Leandro Lupori
2022-06-21 11:05 ` Cédric Le Goater
2022-06-20 20:27 ` [PATCH 2/3] target/ppc: Improve Radix xlate level validation Leandro Lupori
2022-06-20 20:27 ` [PATCH 3/3] target/ppc: Check page dir/table base alignment Leandro Lupori
2 siblings, 1 reply; 12+ messages in thread
From: Leandro Lupori @ 2022-06-20 20:27 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug, Leandro Lupori
Check if partition and process tables are properly aligned, in
their size, according to PowerISA 3.1B, Book III 6.7.6 programming
note. Hardware and KVM also raise an exception in these cases.
Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
hw/ppc/spapr.c | 5 +++++
hw/ppc/spapr_hcall.c | 9 +++++++++
target/ppc/mmu-book3s-v3.c | 5 +++++
target/ppc/mmu-radix64.c | 17 +++++++++++++----
4 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd4942e881..4b1f346087 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1329,6 +1329,11 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
patb = spapr->nested_ptcr & PTCR_PATB;
pats = spapr->nested_ptcr & PTCR_PATS;
+ /* Check if partition table is properly aligned */
+ if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
+ return false;
+ }
+
/* Calculate number of entries */
pats = 1ull << (pats + 12 - 4);
if (pats <= lpid) {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index d761a7d0c3..2a73ba8a1d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -920,6 +920,7 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
target_ulong page_size = args[2];
target_ulong table_size = args[3];
target_ulong update_lpcr = 0;
+ target_ulong table_byte_size;
uint64_t cproc;
if (flags & ~FLAGS_MASK) { /* Check no reserved bits are set */
@@ -927,6 +928,14 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
}
if (flags & FLAG_MODIFY) {
if (flags & FLAG_REGISTER) {
+ /* Check process table alignment */
+ table_byte_size = 1ULL << (table_size + 12);
+ if (proc_tbl & (table_byte_size - 1)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: process table not properly aligned: "
+ "proc_tbl 0x%lx proc_tbl_size 0x%lx\n",
+ __func__, proc_tbl, table_byte_size);
+ }
if (flags & FLAG_RADIX) { /* Register new RADIX process table */
if (proc_tbl & 0xfff || proc_tbl >> 60) {
return H_P2;
diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c
index f4985bae78..c8f69b3df9 100644
--- a/target/ppc/mmu-book3s-v3.c
+++ b/target/ppc/mmu-book3s-v3.c
@@ -28,6 +28,11 @@ bool ppc64_v3_get_pate(PowerPCCPU *cpu, target_ulong lpid, ppc_v3_pate_t *entry)
uint64_t patb = cpu->env.spr[SPR_PTCR] & PTCR_PATB;
uint64_t pats = cpu->env.spr[SPR_PTCR] & PTCR_PATS;
+ /* Check if partition table is properly aligned */
+ if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
+ return false;
+ }
+
/* Calculate number of entries */
pats = 1ull << (pats + 12 - 4);
if (pats <= lpid) {
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 21ac958e48..9a8a2e2875 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -383,7 +383,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
- uint64_t offset, size, prtbe_addr, prtbe0, base_addr, nls, index, pte;
+ uint64_t offset, size, prtb, prtbe_addr, prtbe0, base_addr, nls, index, pte;
int fault_cause = 0, h_page_size, h_prot;
hwaddr h_raddr, pte_addr;
int ret;
@@ -393,9 +393,18 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
__func__, access_str(access_type),
eaddr, mmu_idx, pid);
+ prtb = (pate.dw1 & PATE1_R_PRTB);
+ size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
+ if (prtb & (size - 1)) {
+ /* Process Table not properly aligned */
+ if (guest_visible) {
+ ppc_radix64_raise_si(cpu, access_type, eaddr, DSISR_R_BADCONFIG);
+ }
+ return 1;
+ }
+
/* Index Process Table by PID to Find Corresponding Process Table Entry */
offset = pid * sizeof(struct prtb_entry);
- size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
if (offset >= size) {
/* offset exceeds size of the process table */
if (guest_visible) {
@@ -403,7 +412,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
}
return 1;
}
- prtbe_addr = (pate.dw1 & PATE1_R_PRTB) + offset;
+ prtbe_addr = prtb + offset;
if (vhyp_flat_addressing(cpu)) {
prtbe0 = ldq_phys(cs->as, prtbe_addr);
@@ -568,7 +577,7 @@ static bool ppc_radix64_xlate_impl(PowerPCCPU *cpu, vaddr eaddr,
return false;
}
- /* Get Process Table */
+ /* Get Partition Table */
if (cpu->vhyp) {
PPCVirtualHypervisorClass *vhc;
vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ppc: Check partition and process table alignment
2022-06-20 20:27 ` [PATCH 1/3] ppc: Check partition and process table alignment Leandro Lupori
@ 2022-06-21 11:05 ` Cédric Le Goater
2022-06-23 14:24 ` Leandro Lupori
0 siblings, 1 reply; 12+ messages in thread
From: Cédric Le Goater @ 2022-06-21 11:05 UTC (permalink / raw)
To: Leandro Lupori, qemu-devel, qemu-ppc; +Cc: danielhb413, david, groug
On 6/20/22 22:27, Leandro Lupori wrote:
> Check if partition and process tables are properly aligned, in
> their size, according to PowerISA 3.1B, Book III 6.7.6 programming
> note. Hardware and KVM also raise an exception in these cases.
>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
> hw/ppc/spapr.c | 5 +++++
> hw/ppc/spapr_hcall.c | 9 +++++++++
> target/ppc/mmu-book3s-v3.c | 5 +++++
> target/ppc/mmu-radix64.c | 17 +++++++++++++----
> 4 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e881..4b1f346087 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1329,6 +1329,11 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
> patb = spapr->nested_ptcr & PTCR_PATB;
> pats = spapr->nested_ptcr & PTCR_PATS;
>
> + /* Check if partition table is properly aligned */
> + if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
> + return false;
> + }
> +
> /* Calculate number of entries */
> pats = 1ull << (pats + 12 - 4);
> if (pats <= lpid) {
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index d761a7d0c3..2a73ba8a1d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -920,6 +920,7 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
> target_ulong page_size = args[2];
> target_ulong table_size = args[3];
> target_ulong update_lpcr = 0;
> + target_ulong table_byte_size;
> uint64_t cproc;
>
> if (flags & ~FLAGS_MASK) { /* Check no reserved bits are set */
> @@ -927,6 +928,14 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
> }
> if (flags & FLAG_MODIFY) {
> if (flags & FLAG_REGISTER) {
> + /* Check process table alignment */
> + table_byte_size = 1ULL << (table_size + 12);
> + if (proc_tbl & (table_byte_size - 1)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: process table not properly aligned: "
> + "proc_tbl 0x%lx proc_tbl_size 0x%lx\n",
> + __func__, proc_tbl, table_byte_size);
> + }
I think you might need to use some define for the format. Looks good
otherwise.
Thanks,
C.
> if (flags & FLAG_RADIX) { /* Register new RADIX process table */
> if (proc_tbl & 0xfff || proc_tbl >> 60) {
> return H_P2;
> diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c
> index f4985bae78..c8f69b3df9 100644
> --- a/target/ppc/mmu-book3s-v3.c
> +++ b/target/ppc/mmu-book3s-v3.c
> @@ -28,6 +28,11 @@ bool ppc64_v3_get_pate(PowerPCCPU *cpu, target_ulong lpid, ppc_v3_pate_t *entry)
> uint64_t patb = cpu->env.spr[SPR_PTCR] & PTCR_PATB;
> uint64_t pats = cpu->env.spr[SPR_PTCR] & PTCR_PATS;
>
> + /* Check if partition table is properly aligned */
> + if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
> + return false;
> + }
> +
> /* Calculate number of entries */
> pats = 1ull << (pats + 12 - 4);
> if (pats <= lpid) {
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 21ac958e48..9a8a2e2875 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -383,7 +383,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> - uint64_t offset, size, prtbe_addr, prtbe0, base_addr, nls, index, pte;
> + uint64_t offset, size, prtb, prtbe_addr, prtbe0, base_addr, nls, index, pte;
> int fault_cause = 0, h_page_size, h_prot;
> hwaddr h_raddr, pte_addr;
> int ret;
> @@ -393,9 +393,18 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> __func__, access_str(access_type),
> eaddr, mmu_idx, pid);
>
> + prtb = (pate.dw1 & PATE1_R_PRTB);
> + size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
> + if (prtb & (size - 1)) {
> + /* Process Table not properly aligned */
> + if (guest_visible) {
> + ppc_radix64_raise_si(cpu, access_type, eaddr, DSISR_R_BADCONFIG);
> + }
> + return 1;
> + }
> +
> /* Index Process Table by PID to Find Corresponding Process Table Entry */
> offset = pid * sizeof(struct prtb_entry);
> - size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
> if (offset >= size) {
> /* offset exceeds size of the process table */
> if (guest_visible) {
> @@ -403,7 +412,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> }
> return 1;
> }
> - prtbe_addr = (pate.dw1 & PATE1_R_PRTB) + offset;
> + prtbe_addr = prtb + offset;
>
> if (vhyp_flat_addressing(cpu)) {
> prtbe0 = ldq_phys(cs->as, prtbe_addr);
> @@ -568,7 +577,7 @@ static bool ppc_radix64_xlate_impl(PowerPCCPU *cpu, vaddr eaddr,
> return false;
> }
>
> - /* Get Process Table */
> + /* Get Partition Table */
> if (cpu->vhyp) {
> PPCVirtualHypervisorClass *vhc;
> vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ppc: Check partition and process table alignment
2022-06-21 11:05 ` Cédric Le Goater
@ 2022-06-23 14:24 ` Leandro Lupori
0 siblings, 0 replies; 12+ messages in thread
From: Leandro Lupori @ 2022-06-23 14:24 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, qemu-ppc; +Cc: danielhb413, david, groug
On 6/21/22 08:05, Cédric Le Goater wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você
> possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de
> e-mail suspeito entre imediatamente em contato com o DTI.
>
> On 6/20/22 22:27, Leandro Lupori wrote:
>> Check if partition and process tables are properly aligned, in
>> their size, according to PowerISA 3.1B, Book III 6.7.6 programming
>> note. Hardware and KVM also raise an exception in these cases.
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>> hw/ppc/spapr.c | 5 +++++
>> hw/ppc/spapr_hcall.c | 9 +++++++++
>> target/ppc/mmu-book3s-v3.c | 5 +++++
>> target/ppc/mmu-radix64.c | 17 +++++++++++++----
>> 4 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index fd4942e881..4b1f346087 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1329,6 +1329,11 @@ static bool spapr_get_pate(PPCVirtualHypervisor
>> *vhyp, PowerPCCPU *cpu,
>> patb = spapr->nested_ptcr & PTCR_PATB;
>> pats = spapr->nested_ptcr & PTCR_PATS;
>>
>> + /* Check if partition table is properly aligned */
>> + if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
>> + return false;
>> + }
>> +
>> /* Calculate number of entries */
>> pats = 1ull << (pats + 12 - 4);
>> if (pats <= lpid) {
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index d761a7d0c3..2a73ba8a1d 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -920,6 +920,7 @@ static target_ulong
>> h_register_process_table(PowerPCCPU *cpu,
>> target_ulong page_size = args[2];
>> target_ulong table_size = args[3];
>> target_ulong update_lpcr = 0;
>> + target_ulong table_byte_size;
>> uint64_t cproc;
>>
>> if (flags & ~FLAGS_MASK) { /* Check no reserved bits are set */
>> @@ -927,6 +928,14 @@ static target_ulong
>> h_register_process_table(PowerPCCPU *cpu,
>> }
>> if (flags & FLAG_MODIFY) {
>> if (flags & FLAG_REGISTER) {
>> + /* Check process table alignment */
>> + table_byte_size = 1ULL << (table_size + 12);
>> + if (proc_tbl & (table_byte_size - 1)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: process table not properly aligned: "
>> + "proc_tbl 0x%lx proc_tbl_size 0x%lx\n",
>> + __func__, proc_tbl, table_byte_size);
>> + }
> I think you might need to use some define for the format. Looks good
> otherwise.
>
Right, TARGET_FMT_lx seems more appropriate.
Thanks,
Leandro
> Thanks,
>
> C.
>
>
>> if (flags & FLAG_RADIX) { /* Register new RADIX process
>> table */
>> if (proc_tbl & 0xfff || proc_tbl >> 60) {
>> return H_P2;
>> diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c
>> index f4985bae78..c8f69b3df9 100644
>> --- a/target/ppc/mmu-book3s-v3.c
>> +++ b/target/ppc/mmu-book3s-v3.c
>> @@ -28,6 +28,11 @@ bool ppc64_v3_get_pate(PowerPCCPU *cpu,
>> target_ulong lpid, ppc_v3_pate_t *entry)
>> uint64_t patb = cpu->env.spr[SPR_PTCR] & PTCR_PATB;
>> uint64_t pats = cpu->env.spr[SPR_PTCR] & PTCR_PATS;
>>
>> + /* Check if partition table is properly aligned */
>> + if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
>> + return false;
>> + }
>> +
>> /* Calculate number of entries */
>> pats = 1ull << (pats + 12 - 4);
>> if (pats <= lpid) {
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 21ac958e48..9a8a2e2875 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -383,7 +383,7 @@ static int
>> ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>> {
>> CPUState *cs = CPU(cpu);
>> CPUPPCState *env = &cpu->env;
>> - uint64_t offset, size, prtbe_addr, prtbe0, base_addr, nls, index,
>> pte;
>> + uint64_t offset, size, prtb, prtbe_addr, prtbe0, base_addr, nls,
>> index, pte;
>> int fault_cause = 0, h_page_size, h_prot;
>> hwaddr h_raddr, pte_addr;
>> int ret;
>> @@ -393,9 +393,18 @@ static int
>> ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>> __func__, access_str(access_type),
>> eaddr, mmu_idx, pid);
>>
>> + prtb = (pate.dw1 & PATE1_R_PRTB);
>> + size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
>> + if (prtb & (size - 1)) {
>> + /* Process Table not properly aligned */
>> + if (guest_visible) {
>> + ppc_radix64_raise_si(cpu, access_type, eaddr,
>> DSISR_R_BADCONFIG);
>> + }
>> + return 1;
>> + }
>> +
>> /* Index Process Table by PID to Find Corresponding Process
>> Table Entry */
>> offset = pid * sizeof(struct prtb_entry);
>> - size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
>> if (offset >= size) {
>> /* offset exceeds size of the process table */
>> if (guest_visible) {
>> @@ -403,7 +412,7 @@ static int
>> ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>> }
>> return 1;
>> }
>> - prtbe_addr = (pate.dw1 & PATE1_R_PRTB) + offset;
>> + prtbe_addr = prtb + offset;
>>
>> if (vhyp_flat_addressing(cpu)) {
>> prtbe0 = ldq_phys(cs->as, prtbe_addr);
>> @@ -568,7 +577,7 @@ static bool ppc_radix64_xlate_impl(PowerPCCPU
>> *cpu, vaddr eaddr,
>> return false;
>> }
>>
>> - /* Get Process Table */
>> + /* Get Partition Table */
>> if (cpu->vhyp) {
>> PPCVirtualHypervisorClass *vhc;
>> vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] target/ppc: Improve Radix xlate level validation
2022-06-20 20:27 [PATCH 0/3] ppc: Check for bad Radix configs Leandro Lupori
2022-06-20 20:27 ` [PATCH 1/3] ppc: Check partition and process table alignment Leandro Lupori
@ 2022-06-20 20:27 ` Leandro Lupori
2022-06-21 21:21 ` Fabiano Rosas
2022-06-20 20:27 ` [PATCH 3/3] target/ppc: Check page dir/table base alignment Leandro Lupori
2 siblings, 1 reply; 12+ messages in thread
From: Leandro Lupori @ 2022-06-20 20:27 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug, Leandro Lupori
Check if the number and size of Radix levels are valid on
POWER9/POWER10 CPUs, according to the supported Radix Tree
Configurations described in their User Manuals.
Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
target/ppc/mmu-radix64.c | 36 +++++++++++++++++++++++++++++-------
1 file changed, 29 insertions(+), 7 deletions(-)
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 9a8a2e2875..2f0bcbfe2e 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -236,13 +236,31 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
}
}
+static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
+{
+ /*
+ * Check if this is a valid level, according to POWER9 and POWER10
+ * Processor User's Manuals, sections 4.10.4.1 and 5.10.6.1, respectively:
+ * Supported Radix Tree Configurations and Resulting Page Sizes.
+ */
+ switch (level) {
+ case 0: return psize == 52 && nls == 13; /* Root Page Dir */
+ case 1: return psize == 39 && nls == 9;
+ case 2: return psize == 30 && nls == 9;
+ case 3: return psize == 21 && (nls == 9 || nls == 5);
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "invalid radix level: %d\n", level);
+ return false;
+ }
+}
+
static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
- uint64_t *pte_addr, uint64_t *nls,
+ uint64_t *pte_addr, int *level, uint64_t *nls,
int *psize, uint64_t *pte, int *fault_cause)
{
uint64_t index, pde;
- if (*nls < 5) { /* Directory maps less than 2**5 entries */
+ if (!ppc_radix64_is_valid_level(*level, *psize, *nls)) {
*fault_cause |= DSISR_R_BADCONFIG;
return 1;
}
@@ -257,6 +275,7 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
*pte = pde;
*psize -= *nls;
if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
+ ++*level;
*nls = pde & R_PDE_NLS;
index = eaddr >> (*psize - *nls); /* Shift */
index &= ((1UL << *nls) - 1); /* Mask */
@@ -270,9 +289,10 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
hwaddr *raddr, int *psize, uint64_t *pte,
int *fault_cause, hwaddr *pte_addr)
{
- uint64_t index, pde, rpn , mask;
+ uint64_t index, pde, rpn, mask;
+ int level = 0;
- if (nls < 5) { /* Directory maps less than 2**5 entries */
+ if (!ppc_radix64_is_valid_level(level, *psize, nls)) {
*fault_cause |= DSISR_R_BADCONFIG;
return 1;
}
@@ -283,8 +303,8 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
do {
int ret;
- ret = ppc_radix64_next_level(as, eaddr, pte_addr, &nls, psize, &pde,
- fault_cause);
+ ret = ppc_radix64_next_level(as, eaddr, pte_addr, &level, &nls, psize,
+ &pde, fault_cause);
if (ret) {
return ret;
}
@@ -456,6 +476,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
}
} else {
uint64_t rpn, mask;
+ int level = 0;
index = (eaddr & R_EADDR_MASK) >> (*g_page_size - nls); /* Shift */
index &= ((1UL << nls) - 1); /* Mask */
@@ -476,7 +497,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
}
ret = ppc_radix64_next_level(cs->as, eaddr & R_EADDR_MASK, &h_raddr,
- &nls, g_page_size, &pte, &fault_cause);
+ &level, &nls, g_page_size, &pte,
+ &fault_cause);
if (ret) {
/* No valid pte */
if (guest_visible) {
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] target/ppc: Improve Radix xlate level validation
2022-06-20 20:27 ` [PATCH 2/3] target/ppc: Improve Radix xlate level validation Leandro Lupori
@ 2022-06-21 21:21 ` Fabiano Rosas
2022-06-24 13:22 ` Leandro Lupori
0 siblings, 1 reply; 12+ messages in thread
From: Fabiano Rosas @ 2022-06-21 21:21 UTC (permalink / raw)
To: Leandro Lupori, qemu-devel, qemu-ppc
Cc: clg, danielhb413, david, groug, Leandro Lupori
Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
> Check if the number and size of Radix levels are valid on
> POWER9/POWER10 CPUs, according to the supported Radix Tree
> Configurations described in their User Manuals.
>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
> target/ppc/mmu-radix64.c | 36 +++++++++++++++++++++++++++++-------
> 1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 9a8a2e2875..2f0bcbfe2e 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -236,13 +236,31 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
> }
> }
>
> +static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
I wonder if we should take the time to make this per-CPU now to prepare
for any future CPU that supports a different layout.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] target/ppc: Improve Radix xlate level validation
2022-06-21 21:21 ` Fabiano Rosas
@ 2022-06-24 13:22 ` Leandro Lupori
0 siblings, 0 replies; 12+ messages in thread
From: Leandro Lupori @ 2022-06-24 13:22 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug
On 6/21/22 18:21, Fabiano Rosas wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>
> Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
>
>> Check if the number and size of Radix levels are valid on
>> POWER9/POWER10 CPUs, according to the supported Radix Tree
>> Configurations described in their User Manuals.
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>> target/ppc/mmu-radix64.c | 36 +++++++++++++++++++++++++++++-------
>> 1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 9a8a2e2875..2f0bcbfe2e 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -236,13 +236,31 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
>> }
>> }
>>
>> +static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
>
> I wonder if we should take the time to make this per-CPU now to prepare
> for any future CPU that supports a different layout.
>
The downside would be that we would already take a performance hit by
calling this function through a CPU class pointer. It would also prevent
compiler optimizations that are possible with local static functions.
I can leave a comment in ppc_radix64_is_valid_level, suggesting the
addition of a new CPU class method, if a new CPU that supports other
Radix configurations is added (e.g., Microwatt).
Thanks,
Leandro
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] target/ppc: Check page dir/table base alignment
2022-06-20 20:27 [PATCH 0/3] ppc: Check for bad Radix configs Leandro Lupori
2022-06-20 20:27 ` [PATCH 1/3] ppc: Check partition and process table alignment Leandro Lupori
2022-06-20 20:27 ` [PATCH 2/3] target/ppc: Improve Radix xlate level validation Leandro Lupori
@ 2022-06-20 20:27 ` Leandro Lupori
2022-06-21 21:26 ` Fabiano Rosas
2 siblings, 1 reply; 12+ messages in thread
From: Leandro Lupori @ 2022-06-20 20:27 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug, Leandro Lupori
Check if each page dir/table base address is properly aligned and
log a guest error if not, as real hardware behave incorrectly in
this case.
These checks are only performed when DEBUG_MMU is defined, to avoid
hurting the performance.
Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 2f0bcbfe2e..80d945a7c3 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -28,6 +28,8 @@
#include "mmu-radix64.h"
#include "mmu-book3s-v3.h"
+/* #define DEBUG_MMU */
+
static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
vaddr eaddr,
uint64_t *lpid, uint64_t *pid)
@@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
++*level;
*nls = pde & R_PDE_NLS;
+
+#ifdef DEBUG_MMU
+ if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
+ " page dir size: 0x%"PRIx64" level: %d\n",
+ __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level);
+ }
+#endif
+
index = eaddr >> (*psize - *nls); /* Shift */
index &= ((1UL << *nls) - 1); /* Mask */
*pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
@@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
return 1;
}
+#ifdef DEBUG_MMU
+ if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: misaligned page dir base: 0x%"VADDR_PRIx
+ " page dir size: 0x%"PRIx64"\n",
+ __func__, base_addr, BIT(nls + 3));
+ }
+#endif
+
index = eaddr >> (*psize - nls); /* Shift */
index &= ((1UL << nls) - 1); /* Mask */
*pte_addr = base_addr + (index * sizeof(pde));
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/ppc: Check page dir/table base alignment
2022-06-20 20:27 ` [PATCH 3/3] target/ppc: Check page dir/table base alignment Leandro Lupori
@ 2022-06-21 21:26 ` Fabiano Rosas
2022-06-23 14:26 ` Leandro Lupori
0 siblings, 1 reply; 12+ messages in thread
From: Fabiano Rosas @ 2022-06-21 21:26 UTC (permalink / raw)
To: Leandro Lupori, qemu-devel, qemu-ppc
Cc: clg, danielhb413, david, groug, Leandro Lupori
Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
> Check if each page dir/table base address is properly aligned and
> log a guest error if not, as real hardware behave incorrectly in
> this case.
>
> These checks are only performed when DEBUG_MMU is defined, to avoid
> hurting the performance.
>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 2f0bcbfe2e..80d945a7c3 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -28,6 +28,8 @@
> #include "mmu-radix64.h"
> #include "mmu-book3s-v3.h"
>
> +/* #define DEBUG_MMU */
> +
> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
> vaddr eaddr,
> uint64_t *lpid, uint64_t *pid)
> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
> ++*level;
> *nls = pde & R_PDE_NLS;
> +
> +#ifdef DEBUG_MMU
> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
> + " page dir size: 0x%"PRIx64" level: %d\n",
> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level);
> + }
> +#endif
Maybe use qemu_log_enabled() instead of the define? I think it is a
little more useful and has less chance to rot.
> +
> index = eaddr >> (*psize - *nls); /* Shift */
> index &= ((1UL << *nls) - 1); /* Mask */
> *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
> @@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
> return 1;
> }
>
> +#ifdef DEBUG_MMU
> + if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: misaligned page dir base: 0x%"VADDR_PRIx
> + " page dir size: 0x%"PRIx64"\n",
> + __func__, base_addr, BIT(nls + 3));
> + }
> +#endif
> +
> index = eaddr >> (*psize - nls); /* Shift */
> index &= ((1UL << nls) - 1); /* Mask */
> *pte_addr = base_addr + (index * sizeof(pde));
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/ppc: Check page dir/table base alignment
2022-06-21 21:26 ` Fabiano Rosas
@ 2022-06-23 14:26 ` Leandro Lupori
2022-06-23 21:34 ` Richard Henderson
0 siblings, 1 reply; 12+ messages in thread
From: Leandro Lupori @ 2022-06-23 14:26 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug
On 6/21/22 18:26, Fabiano Rosas wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>
> Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
>
>> Check if each page dir/table base address is properly aligned and
>> log a guest error if not, as real hardware behave incorrectly in
>> this case.
>>
>> These checks are only performed when DEBUG_MMU is defined, to avoid
>> hurting the performance.
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 2f0bcbfe2e..80d945a7c3 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -28,6 +28,8 @@
>> #include "mmu-radix64.h"
>> #include "mmu-book3s-v3.h"
>>
>> +/* #define DEBUG_MMU */
>> +
>> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
>> vaddr eaddr,
>> uint64_t *lpid, uint64_t *pid)
>> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
>> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>> ++*level;
>> *nls = pde & R_PDE_NLS;
>> +
>> +#ifdef DEBUG_MMU
>> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
>> + " page dir size: 0x%"PRIx64" level: %d\n",
>> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level);
>> + }
>> +#endif
>
> Maybe use qemu_log_enabled() instead of the define? I think it is a
> little more useful and has less chance to rot.
>
Ok. I wanted to avoid introducing any extra overhead, but I guess just
checking qemu_log_enabled() should be ok.
>> +
>> index = eaddr >> (*psize - *nls); /* Shift */
>> index &= ((1UL << *nls) - 1); /* Mask */
>> *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
>> @@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
>> return 1;
>> }
>>
>> +#ifdef DEBUG_MMU
>> + if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: misaligned page dir base: 0x%"VADDR_PRIx
>> + " page dir size: 0x%"PRIx64"\n",
>> + __func__, base_addr, BIT(nls + 3));
>> + }
>> +#endif
>> +
>> index = eaddr >> (*psize - nls); /* Shift */
>> index &= ((1UL << nls) - 1); /* Mask */
>> *pte_addr = base_addr + (index * sizeof(pde));
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/ppc: Check page dir/table base alignment
2022-06-23 14:26 ` Leandro Lupori
@ 2022-06-23 21:34 ` Richard Henderson
2022-06-24 12:20 ` Leandro Lupori
0 siblings, 1 reply; 12+ messages in thread
From: Richard Henderson @ 2022-06-23 21:34 UTC (permalink / raw)
To: Leandro Lupori, Fabiano Rosas, qemu-devel, qemu-ppc
Cc: clg, danielhb413, david, groug
On 6/23/22 07:26, Leandro Lupori wrote:
> On 6/21/22 18:26, Fabiano Rosas wrote:
>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o
>> remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre
>> imediatamente em contato com o DTI.
>>
>> Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
>>
>>> Check if each page dir/table base address is properly aligned and
>>> log a guest error if not, as real hardware behave incorrectly in
>>> this case.
>>>
>>> These checks are only performed when DEBUG_MMU is defined, to avoid
>>> hurting the performance.
>>>
>>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>>> ---
>>> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>>> index 2f0bcbfe2e..80d945a7c3 100644
>>> --- a/target/ppc/mmu-radix64.c
>>> +++ b/target/ppc/mmu-radix64.c
>>> @@ -28,6 +28,8 @@
>>> #include "mmu-radix64.h"
>>> #include "mmu-book3s-v3.h"
>>>
>>> +/* #define DEBUG_MMU */
>>> +
>>> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
>>> vaddr eaddr,
>>> uint64_t *lpid, uint64_t *pid)
>>> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
>>> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>>> ++*level;
>>> *nls = pde & R_PDE_NLS;
>>> +
>>> +#ifdef DEBUG_MMU
>>> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
>>> + " page dir size: 0x%"PRIx64" level: %d\n",
>>> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level);
>>> + }
>>> +#endif
>>
>> Maybe use qemu_log_enabled() instead of the define? I think it is a
>> little more useful and has less chance to rot.
>>
>
> Ok. I wanted to avoid introducing any extra overhead, but I guess just checking
> qemu_log_enabled() should be ok.
No, qemu_log_enabled is *already* taken into account by qemu_log_mask.
Just drop the ifdefs.
Do you in fact need to raise an exception here?
r~
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] target/ppc: Check page dir/table base alignment
2022-06-23 21:34 ` Richard Henderson
@ 2022-06-24 12:20 ` Leandro Lupori
0 siblings, 0 replies; 12+ messages in thread
From: Leandro Lupori @ 2022-06-24 12:20 UTC (permalink / raw)
To: Richard Henderson, Fabiano Rosas, qemu-devel, qemu-ppc
Cc: clg, danielhb413, david, groug
On 6/23/22 18:34, Richard Henderson wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você
> possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de
> e-mail suspeito entre imediatamente em contato com o DTI.
>
> On 6/23/22 07:26, Leandro Lupori wrote:
>> On 6/21/22 18:26, Fabiano Rosas wrote:
>>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você
>>> possa confirmar o
>>> remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito
>>> entre
>>> imediatamente em contato com o DTI.
>>>
>>> Leandro Lupori <leandro.lupori@eldorado.org.br> writes:
>>>
>>>> Check if each page dir/table base address is properly aligned and
>>>> log a guest error if not, as real hardware behave incorrectly in
>>>> this case.
>>>>
>>>> These checks are only performed when DEBUG_MMU is defined, to avoid
>>>> hurting the performance.
>>>>
>>>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>>>> ---
>>>> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++
>>>> 1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>>>> index 2f0bcbfe2e..80d945a7c3 100644
>>>> --- a/target/ppc/mmu-radix64.c
>>>> +++ b/target/ppc/mmu-radix64.c
>>>> @@ -28,6 +28,8 @@
>>>> #include "mmu-radix64.h"
>>>> #include "mmu-book3s-v3.h"
>>>>
>>>> +/* #define DEBUG_MMU */
>>>> +
>>>> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState
>>>> *env,
>>>> vaddr eaddr,
>>>> uint64_t *lpid,
>>>> uint64_t *pid)
>>>> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace
>>>> *as, vaddr eaddr,
>>>> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>>>> ++*level;
>>>> *nls = pde & R_PDE_NLS;
>>>> +
>>>> +#ifdef DEBUG_MMU
>>>> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
>>>> + " page dir size: 0x%"PRIx64" level: %d\n",
>>>> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level);
>>>> + }
>>>> +#endif
>>>
>>> Maybe use qemu_log_enabled() instead of the define? I think it is a
>>> little more useful and has less chance to rot.
>>>
>>
>> Ok. I wanted to avoid introducing any extra overhead, but I guess just
>> checking
>> qemu_log_enabled() should be ok.
>
> No, qemu_log_enabled is *already* taken into account by qemu_log_mask.
> Just drop the ifdefs.
>
> Do you in fact need to raise an exception here?
>
Not in this case. I've tested it with KVM and it doesn't raise an
exception. It seems to just ignore PDE_NLB's bits lower than nls + 3.
Thanks,
Leandro
>
> r~
^ permalink raw reply [flat|nested] 12+ messages in thread