[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/7] arm/mpu: Provide access to the MPU region from the C code
- To: Luca Fancellu <Luca.Fancellu@xxxxxxx>
- From: Ayan Kumar Halder <ayankuma@xxxxxxx>
- Date: Wed, 9 Apr 2025 11:46:23 +0100
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=RXjeDSjKaKUBSlhXupgJQX6UkrOCSr4+ro7Z7dI/9/8=; b=MDzYMdCuCS/kKiJZl6YcLpnH9HZ1jk1/5AeCD0pLJiUp8KANEXoYdeAPA6u63+9StXjFrPy8MOW53ivQn/XNll2nn8dczllMd8IH0Lfw979r+yqfvfGf4QUvKwUgRaXX+XvoeQUYzJADoMvv2n2tJ/pnjbGhFFigF4iAboyw4fJxRbcmMXOyaKmhzwuFnseBJxOdeTWy050J5o70WEBC2De8jB6Bl7b+bMjuGcYHQbuXASG8gEoR47tFVNXUkZyeFgpC1kIU+aGnMgFldF2ccRhqoE2skAuQLZ1luA1+V+iYGk0t2iWxGVMUW9rN4FlHUcwteKHh3pIBqbxuyWU9Ug==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=qqrsYvM73oEw0BC70P8oBdzZvkzl0Wy10VK+XqJTgRIcqpjuxfCiYtUcUgRH+nxAT15lLT4af7/jVsO7lsQuD8nYTWOKP3ibIPEikE09vuUh+TBpKzTmGwj69y8CS7y8VpoaLsrKGgRcD9KCBSujwDjt64ZDCZBPk5I593jR/4g5HLaT0WLSgLzaGcrlqiwmq29/zu3nQw0EkMdqeDOF+uHgYmvnZoq8esGL3gaiH7paUNtCEZCOtey72pmFB6CGJaz84XcprbRPPHeTZ1ghEowXGdsaGA9omy1MPEmPP9D/O5DPUY2EFzVEadkcOsqAKX9kMxoe7UlJGxsVtejVyg==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
- Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
- Delivery-date: Wed, 09 Apr 2025 10:46:41 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 09/04/2025 11:21, Luca Fancellu wrote:
Hi Ayan,
Hi,
On 9 Apr 2025, at 11:07, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
On 09/04/2025 09:26, Luca Fancellu wrote:
Hi Ayan,
Hi Luca,
The point of the code was to don’t issue an isb() every time we change the
selector,
of course the code would be easier otherwise, but do we want to do that?
Not sure if it is beneficial as you would need to use isb() from region16
onwards.
The isb() is issued only when changing the selector, so when you go from
reading/writing regions
from 0-15 to 16-31 for example, of course there is a case that if you
continuously write on region 15
and 16 you would have to always change the selector, but it’s the less impact
we could have.
armv8-R is even better since it’s able to address regions from 0 to 23 without
flushing the pipeline,
^— aarch32 :)
Can you point me to the document where you got this info ?
was referring to the r52, of course you are more knowledgeable on the armv8-r
aarch32 part, so ...
If you can point me to the page and section within R52 TRM, I can take a
look to see if I missed something (which isn't suprising as the docs are
huge :))
so I would say we should exploit this big advantage.
I’ll send shortly in this thread the code I would use and the code I was
thinking you could use.
Here the code I have in mind:
static void prepare_selector(uint8_t *sel)
{
uint8_t cur_sel = *sel;
/*
* {read,write}_protection_region works using the direct access to the
0..15
* regions, so in order to save the isb() overhead, change the PRSELR_EL2
* only when needed, so when the upper 4 bits of the selector will change.
*/
cur_sel &= 0xF0U;
if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
{
WRITE_SYSREG(cur_sel, PRSELR_EL2);
isb();
}
*sel = *sel & 0xFU;
}
void read_protection_region(pr_t *pr_read, uint8_t sel)
{
/*
* Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
* make sure PRSELR_EL2 is set, as it determines which MPU region
* is selected.
*/
prepare_selector(&sel);
switch ( sel )
{
GENERATE_READ_PR_REG_CASE(0, pr_read);
GENERATE_READ_PR_REG_CASE(1, pr_read);
GENERATE_READ_PR_REG_CASE(2, pr_read);
GENERATE_READ_PR_REG_CASE(3, pr_read);
GENERATE_READ_PR_REG_CASE(4, pr_read);
GENERATE_READ_PR_REG_CASE(5, pr_read);
GENERATE_READ_PR_REG_CASE(6, pr_read);
GENERATE_READ_PR_REG_CASE(7, pr_read);
GENERATE_READ_PR_REG_CASE(8, pr_read);
GENERATE_READ_PR_REG_CASE(9, pr_read);
GENERATE_READ_PR_REG_CASE(10, pr_read);
GENERATE_READ_PR_REG_CASE(11, pr_read);
GENERATE_READ_PR_REG_CASE(12, pr_read);
GENERATE_READ_PR_REG_CASE(13, pr_read);
GENERATE_READ_PR_REG_CASE(14, pr_read);
GENERATE_READ_PR_REG_CASE(15, pr_read);
default:
BUG(); /* Can't happen */
}
}
void write_protection_region(const pr_t *pr_write, uint8_t sel)
{
/*
* Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
* make sure PRSELR_EL2 is set, as it determines which MPU region
* is selected.
*/
prepare_selector(&sel);
switch ( sel )
{
GENERATE_WRITE_PR_REG_CASE(0, pr_write);
GENERATE_WRITE_PR_REG_CASE(1, pr_write);
GENERATE_WRITE_PR_REG_CASE(2, pr_write);
GENERATE_WRITE_PR_REG_CASE(3, pr_write);
GENERATE_WRITE_PR_REG_CASE(4, pr_write);
GENERATE_WRITE_PR_REG_CASE(5, pr_write);
GENERATE_WRITE_PR_REG_CASE(6, pr_write);
GENERATE_WRITE_PR_REG_CASE(7, pr_write);
GENERATE_WRITE_PR_REG_CASE(8, pr_write);
GENERATE_WRITE_PR_REG_CASE(9, pr_write);
GENERATE_WRITE_PR_REG_CASE(10, pr_write);
GENERATE_WRITE_PR_REG_CASE(11, pr_write);
GENERATE_WRITE_PR_REG_CASE(12, pr_write);
GENERATE_WRITE_PR_REG_CASE(13, pr_write);
GENERATE_WRITE_PR_REG_CASE(14, pr_write);
GENERATE_WRITE_PR_REG_CASE(15, pr_write);
default:
BUG(); /* Can't happen */
}
}
Till here I am fine if you think this is the correct approach for arm64.
Ok
Here the code I thought you could add for arm32:
static void prepare_selector(uint8_t *sel)
{
uint8_t cur_sel = *sel;
#ifdef CONFIG_ARM_64
/*
* {read,write}_protection_region works using the direct access to the
0..15
* regions, so in order to save the isb() overhead, change the PRSELR_EL2
* only when needed, so when the upper 4 bits of the selector will change.
*/
cur_sel &= 0xF0U;
if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
{
WRITE_SYSREG(cur_sel, PRSELR_EL2);
isb();
}
*sel = *sel & 0xFU;
#else
if ( cur_sel > 23 )
panic("Armv8-R AArch32 region selector exceeds maximum allowed
range!");
I am not sure about this. See my question before. However ...
#endif
}
From ARM DDI 0568A.c ID110520
E2.2.3 HPRBAR<n> - Provides access to the base addresses for the first 32
defined EL2 MPU regions.
E2.2.6 HPRLAR<n> - Provides access to the limit addresses for the first 32
defined EL2 MPU regions.
I understand that HPRSELR is not used when directly accessing the above two
registers. Thus, this function will be a nop for Arm32. Please let me know if I
am mistaken.
yes you are right, you can decide if doing something or not in that function, you
don’t need to change the selector.
void read_protection_region(pr_t *pr_read, uint8_t sel)
{
/*
* Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
* make sure PRSELR_EL2 is set, as it determines which MPU region
* is selected.
*/
prepare_selector(&sel);
switch ( sel )
{
GENERATE_READ_PR_REG_CASE(0, pr_read);
GENERATE_READ_PR_REG_CASE(1, pr_read);
GENERATE_READ_PR_REG_CASE(2, pr_read);
GENERATE_READ_PR_REG_CASE(3, pr_read);
GENERATE_READ_PR_REG_CASE(4, pr_read);
GENERATE_READ_PR_REG_CASE(5, pr_read);
GENERATE_READ_PR_REG_CASE(6, pr_read);
GENERATE_READ_PR_REG_CASE(7, pr_read);
GENERATE_READ_PR_REG_CASE(8, pr_read);
GENERATE_READ_PR_REG_CASE(9, pr_read);
GENERATE_READ_PR_REG_CASE(10, pr_read);
GENERATE_READ_PR_REG_CASE(11, pr_read);
GENERATE_READ_PR_REG_CASE(12, pr_read);
GENERATE_READ_PR_REG_CASE(13, pr_read);
GENERATE_READ_PR_REG_CASE(14, pr_read);
GENERATE_READ_PR_REG_CASE(15, pr_read);
#ifdef CONFIG_ARM_32
GENERATE_READ_PR_REG_CASE(16, pr_read);
GENERATE_READ_PR_REG_CASE(17, pr_read);
GENERATE_READ_PR_REG_CASE(18, pr_read);
GENERATE_READ_PR_REG_CASE(19, pr_read);
GENERATE_READ_PR_REG_CASE(20, pr_read);
GENERATE_READ_PR_REG_CASE(21, pr_read);
GENERATE_READ_PR_REG_CASE(22, pr_read);
GENERATE_READ_PR_REG_CASE(23, pr_read);
At least 32 regions are directly accessible, thus thisn should go till 31
(0-31). Same ..
yeah you can add until the 31st here and ...
#endif
default:
BUG(); /* Can't happen */
}
}
void write_protection_region(const pr_t *pr_write, uint8_t sel)
{
/*
* Before accessing EL2 MPU region register PRBAR_EL2/PRLAR_EL2,
* make sure PRSELR_EL2 is set, as it determines which MPU region
* is selected.
*/
prepare_selector(&sel);
switch ( sel )
{
GENERATE_WRITE_PR_REG_CASE(0, pr_write);
GENERATE_WRITE_PR_REG_CASE(1, pr_write);
GENERATE_WRITE_PR_REG_CASE(2, pr_write);
GENERATE_WRITE_PR_REG_CASE(3, pr_write);
GENERATE_WRITE_PR_REG_CASE(4, pr_write);
GENERATE_WRITE_PR_REG_CASE(5, pr_write);
GENERATE_WRITE_PR_REG_CASE(6, pr_write);
GENERATE_WRITE_PR_REG_CASE(7, pr_write);
GENERATE_WRITE_PR_REG_CASE(8, pr_write);
GENERATE_WRITE_PR_REG_CASE(9, pr_write);
GENERATE_WRITE_PR_REG_CASE(10, pr_write);
GENERATE_WRITE_PR_REG_CASE(11, pr_write);
GENERATE_WRITE_PR_REG_CASE(12, pr_write);
GENERATE_WRITE_PR_REG_CASE(13, pr_write);
GENERATE_WRITE_PR_REG_CASE(14, pr_write);
GENERATE_WRITE_PR_REG_CASE(15, pr_write);
#ifdef CONFIG_ARM_32
GENERATE_WRITE_PR_REG_CASE(16, pr_write);
GENERATE_WRITE_PR_REG_CASE(17, pr_write);
GENERATE_WRITE_PR_REG_CASE(18, pr_write);
GENERATE_WRITE_PR_REG_CASE(19, pr_write);
GENERATE_WRITE_PR_REG_CASE(20, pr_write);
GENERATE_WRITE_PR_REG_CASE(21, pr_write);
GENERATE_WRITE_PR_REG_CASE(22, pr_write);
GENERATE_WRITE_PR_REG_CASE(23, pr_write);
for here.
also here.
So I guess we are aligned for this patch right? I will update this patch as the
code above and you
will modify the code for arm32 to support the direct access up to 32 region. Ok?
This is ok.
- Ayan
Cheers,
Luca
|
